Add thread archive CLI commands#25021
Conversation
74ee516 to
933e434
Compare
933e434 to
629f003
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 629f003310
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1aa6890 to
f1049e1
Compare
Problem: Saved threads can be archived through app-server RPCs, but the command line has no direct archive or unarchive entrypoint. Solution: Add codex archive and codex unarchive commands that resolve thread UUIDs or exact names and call the existing app-server archive RPCs.
f1049e1 to
5e63b78
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e63b786ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34a90a0c77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efc80b1022
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| archived: Some(matches!(action, SessionArchiveAction::Unarchive)), | ||
| cwd: None, | ||
| use_state_db_only: false, | ||
| search_term: None, |
There was a problem hiding this comment.
Filter archive name lookups server-side
When codex archive <name> or codex unarchive <name> resolves a name, this omits search_term, so the loop pages through every active/archived session until it finds an exact match or exhausts history. For users with large or repaired rollout histories—especially a missing archived name—this can force a full scan/repair of the session store instead of using the app-server title filter already used by resume name lookup; pass search_term: Some(name.to_string()) here and keep the exact-name check on the filtered results.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a good catch. It should be consistent with codex resume behavior. Fixed.
fcoury-oai
left a comment
There was a problem hiding this comment.
Exercised the happy path and it worked as expected. The code looks good as well.
Codex found 3 issues, 2 are non-blocking in my opinion. You may want to address the duplicate session name one, since this mutates the session as explained before.
I will approve this and leave the decision of which issues to address up to you.
| limit: Some(100), | ||
| sort_key: Some(ThreadSortKey::UpdatedAt), | ||
| sort_direction: None, | ||
| model_providers: None, |
There was a problem hiding this comment.
Issue found by Codex below. In my opinion this can either be addressed here or as a separate PR. Non-blocking.
model_providers: None is interpreted by app-server as the current configured provider only. The archive command accepts and merges --oss / --local-provider, but its config bootstrap never applies those provider overrides. Name-based archive and unarchive therefore cannot find sessions from another provider, even when the user explicitly supplies the matching OSS flags. UUID lookup still works.
There was a problem hiding this comment.
I'm going to leave this as is because it's consistent with how codex resume works. If we decide to change this, we should do so everywhere.
| .await | ||
| .wrap_err("failed to list sessions while resolving session name")?; | ||
|
|
||
| if let Some(thread) = response |
There was a problem hiding this comment.
Session names are not unique, but lookup returns the first exact match sorted by updatedAt. Resume has similar lookup behavior, but archive is a storage mutation. Picking one silently can archive or unarchive the wrong session.
There was a problem hiding this comment.
Yeah, I thought about that. If /archive was non-reversible, I'd feel more strongly about this. Since it's easily reversible, I think this is reasonable. The alternative is to 1) not allow name lookup, 2) do a full (potentially slow) scan for duplicate names and ask the user for confirmation.
| archived: Some(matches!(action, SessionArchiveAction::Unarchive)), | ||
| cwd: None, | ||
| use_state_db_only: false, | ||
| search_term: None, |
There was a problem hiding this comment.
Another non-blocking issue: archive commands can become slow for users with large histories. The reason I think this can be handled as a separate PR is because fork and resume exhibit the same issue. We should eventually fix this behavior.
Unlike the existing resume-by-name path, this sends search_term: None and pages through the complete active or archived history. That creates unnecessary RPCs and read-repair work, especially for missing names.
There was a problem hiding this comment.
Yes, I agree this should not be tackled in this PR.
## Why TUI users can archive saved sessions from other surfaces, but there is no in-session command for archiving the active session. Since archiving the active session also exits the TUI, the command should ask for explicit confirmation instead of firing immediately. I'm also working on [a companion PR](#25021) that adds `codex archive` and `codex unarchive` top-level CLI commands. ## What changed - Adds a new `/archive` slash command described as `archive this session and exit`. - Shows a confirmation dialog with `No, don't archive` selected first and `Yes, archive and exit` as the explicit action. - On confirmation, calls the existing `thread/archive` app-server RPC for the active main session and exits after success. - Keeps `/archive` disabled while a task is running and unavailable in side conversations. ## Verification Added focused TUI coverage for the `/archive` confirmation flow, disabled-while-task-running behavior, and the `/ar` slash-command popup snapshot.
# Conflicts: # codex-rs/tui/src/app_server_session.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17f9eab872
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6a1109595
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if stored_thread.archived_at.is_some() { | ||
| let thread_id = stored_thread.thread_id; | ||
| return Err(invalid_request(format!( | ||
| "session {thread_id} is archived. Run `codex unarchive {thread_id}` to unarchive it first." | ||
| ))); |
There was a problem hiding this comment.
Allow explicit forks from archived sessions
When a user explicitly forks an archived session by id or rollout path, this new guard rejects the request before thread_fork_inner can read the stored history, so codex fork <id> and thread/fork require an unnecessary unarchive/re-archive round trip even though forking only needs the archived transcript as a source. Fresh evidence after the prior comment: in this revision thread_fork_inner still calls this same read_stored_thread_for_resume helper, so the new archived_at check applies to fork as well as resume.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is intended.
Problem
Saved threads can already be archived through app-server RPCs, but the command line did not expose direct archive or unarchive commands.
Solution
Add
codex archive <thread>andcodex unarchive <thread>, resolving UUIDs or exact thread names before calling the existingthread/archiveandthread/unarchiveRPCs. The commands support scoped remote flags so callers can target remote app-server endpoints when archiving or unarchiving threads.This also fixes a long-standing bug in
codex resume <thread id>andcodex fork <thread id>that I found when testing the new commands. These operations shouldn't be allowed on archived sessions. They now fail with an error that tells the user to runcodex unarchive <thread id>first.Verification
Added app-server coverage for rejecting archived thread resume by id and checking that the error includes the matching
codex unarchive <thread id>command.